-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clang][PAC] ptrauth_qualifier and ptrauth_intrinsic should only be available on Darwin #153912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][PAC] ptrauth_qualifier and ptrauth_intrinsic should only be available on Darwin #153912
Conversation
… features For backwards compatibility reasons the ptrauth_qualifier and ptrauth_intrinsic features need to be testable with __has_feature() on Apple platforms, but for other platforms this backwards compatibility issue does not exist. This PR resolves these issues by making the ptrauth_qualifier and ptrauth_intrinsic tests conditional upon a darwin target. This also allows us to revert the ptrauth_qualifier check from an extension to a feature test again, as is required on these platforms. At the same time we introduce a new predefined macro __PTRAUTH__ that answers the same question as __has_feature(ptrauth_qualifier) and __has_feature(ptrauth_intrinsic) as those tests are synonymous and only exist separately for compatibility reasons. The requirement to test for the __PTRAUTH__ macro also resolves the hazard presented by mixing the ptrauth_qualifier flag (that impacts ABI and security policies) with -pedantics-errors, which makes __has_extension return false for all extensions.
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesFor backwards compatibility reasons the ptrauth_qualifier and ptrauth_intrinsic features need to be testable with __has_feature() on Apple platforms, but for other platforms this backwards compatibility issue does not exist. This PR resolves these issues by making the ptrauth_qualifier and ptrauth_intrinsic tests conditional upon a darwin target. This also allows us to revert the ptrauth_qualifier check from an extension to a feature test again, as is required on these platforms. At the same time we introduce a new predefined macro PTRAUTH that answers the same question as __has_feature(ptrauth_qualifier) and __has_feature(ptrauth_intrinsic) as those tests are synonymous and only exist separately for compatibility reasons. The requirement to test for the PTRAUTH macro also resolves the hazard presented by mixing the ptrauth_qualifier flag (that impacts ABI and security policies) with -pedantics-errors, which makes __has_extension return false for all extensions. Full diff: https://github.com/llvm/llvm-project/pull/153912.diff 9 Files Affected:
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index b9efc6a6a2e9d..6f414105d7c36 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -147,8 +147,8 @@ FEATURE(type_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Type))
FEATURE(thread_sanitizer, LangOpts.Sanitize.has(SanitizerKind::Thread))
FEATURE(dataflow_sanitizer, LangOpts.Sanitize.has(SanitizerKind::DataFlow))
FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
-FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics)
-EXTENSION(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics)
+FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics && Target.getTriple().isOSDarwin())
+FEATURE(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics && Target.getTriple().isOSDarwin())
FEATURE(ptrauth_calls, LangOpts.PointerAuthCalls)
FEATURE(ptrauth_returns, LangOpts.PointerAuthReturns)
FEATURE(ptrauth_vtable_pointer_address_discrimination, LangOpts.PointerAuthVTPtrAddressDiscrimination)
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 685a9bbf2cde9..d29a08c43fc12 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -1159,6 +1159,7 @@ void DumpCompilerOptionsAction::ExecuteAction() {
raw_ostream &OS = *OSP;
const Preprocessor &PP = CI.getPreprocessor();
const LangOptions &LangOpts = PP.getLangOpts();
+ const TargetInfo &Target = CI.getTarget();
// FIXME: Rather than manually format the JSON (which is awkward due to
// needing to remove trailing commas), this should make use of a JSON library.
diff --git a/clang/lib/Frontend/InitPreprocessor.cpp b/clang/lib/Frontend/InitPreprocessor.cpp
index 008a35d5265e1..accddfebc0f6f 100644
--- a/clang/lib/Frontend/InitPreprocessor.cpp
+++ b/clang/lib/Frontend/InitPreprocessor.cpp
@@ -1528,6 +1528,9 @@ static void InitializePredefinedMacros(const TargetInfo &TI,
#undef TARGET_OS
}
+ if (LangOpts.PointerAuthIntrinsics)
+ Builder.defineMacro("__PTRAUTH__");
+
// Get other target #defines.
TI.getTargetDefines(LangOpts, Builder);
}
diff --git a/clang/lib/Headers/ptrauth.h b/clang/lib/Headers/ptrauth.h
index 7f7d387cbdfda..f902ca1e3bbd3 100644
--- a/clang/lib/Headers/ptrauth.h
+++ b/clang/lib/Headers/ptrauth.h
@@ -95,7 +95,7 @@ typedef __UINTPTR_TYPE__ ptrauth_generic_signature_t;
__ptrauth qualifier; the compiler will perform this check
automatically. */
-#if __has_feature(ptrauth_intrinsics)
+#if __has_feature(ptrauth_intrinsics) || defined(__PTRAUTH__)
/* Strip the signature from a value without authenticating it.
@@ -388,6 +388,6 @@ typedef __UINTPTR_TYPE__ ptrauth_generic_signature_t;
#define __ptrauth_objc_isa_uintptr
#define __ptrauth_objc_super_pointer
-#endif /* __has_feature(ptrauth_intrinsics) */
+#endif /* __has_feature(ptrauth_intrinsics) || defined(__PTRAUTH__) */
#endif /* __PTRAUTH_H */
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 6f12ac80d677e..f9dfd0eb14bc0 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1089,6 +1089,7 @@ static void ComputeDATE_TIME(SourceLocation &DATELoc, SourceLocation &TIMELoc,
/// specified by the identifier as a standard language feature.
static bool HasFeature(const Preprocessor &PP, StringRef Feature) {
const LangOptions &LangOpts = PP.getLangOpts();
+ const TargetInfo &Target = PP.getTargetInfo();
// Normalize the feature name, __foo__ becomes foo.
if (Feature.starts_with("__") && Feature.ends_with("__") &&
diff --git a/clang/test/Preprocessor/ptrauth_extension.c b/clang/test/Preprocessor/ptrauth_extension.c
index d6b79187ba62d..3267b0786c28f 100644
--- a/clang/test/Preprocessor/ptrauth_extension.c
+++ b/clang/test/Preprocessor/ptrauth_extension.c
@@ -4,10 +4,32 @@
// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-calls | \
// RUN: FileCheck %s --check-prefixes=NOINTRIN
-#if __has_extension(ptrauth_qualifier)
-// INTRIN: has_ptrauth_qualifier
-void has_ptrauth_qualifier() {}
-#else
+// RUN: %clang_cc1 -E %s -DIS_DARWIN -triple=arm64e-apple-darwin -fptrauth-intrinsics | \
+// RUN: FileCheck %s --check-prefixes=INTRIN,INTRIN_MAC
+
+// RUN: %clang_cc1 -E %s -DIS_DARWIN -triple=arm64e-apple-darwin -fptrauth-calls | \
+// RUN: FileCheck %s --check-prefixes=NOINTRIN
+
+#if defined(IS_DARWIN) && __has_extension(ptrauth_qualifier)
+// INTRIN_MAC: has_ptrauth_qualifier1
+void has_ptrauth_qualifier1() {}
+#ifndef __PTRAUTH__
+#error ptrauth_qualifier extension present without predefined test macro
+#endif
+#endif
+#if defined(IS_DARWIN) && __has_feature(ptrauth_qualifier)
+// INTRIN_MAC: has_ptrauth_qualifier2
+void has_ptrauth_qualifier2() {}
+#ifndef __PTRAUTH__
+#error ptrauth_qualifier extension present without predefined test macro
+#endif
+#endif
+#if defined(__PTRAUTH__)
+// INTRIN: has_ptrauth_qualifier3
+void has_ptrauth_qualifier3() {}
+#endif
+
+#if !defined(__PTRAUTH__) && !__has_feature(ptrauth_qualifier) && !__has_extension(ptrauth_qualifier)
// NOINTRIN: no_ptrauth_qualifier
void no_ptrauth_qualifier() {}
#endif
diff --git a/clang/test/Preprocessor/ptrauth_feature.c b/clang/test/Preprocessor/ptrauth_feature.c
index a440791d6cc69..45d9cd4245dba 100644
--- a/clang/test/Preprocessor/ptrauth_feature.c
+++ b/clang/test/Preprocessor/ptrauth_feature.c
@@ -34,7 +34,7 @@
// RUN: %clang_cc1 -E %s -triple=aarch64 -fptrauth-elf-got | \
// RUN: FileCheck %s --check-prefixes=NOINTRIN,NOCALLS,NORETS,NOVPTR_ADDR_DISCR,NOVPTR_TYPE_DISCR,NOTYPE_INFO_DISCR,NOFUNC,NOINITFINI,NOINITFINI_ADDR_DISCR,NOGOTOS,ELFGOT
-#if __has_feature(ptrauth_intrinsics)
+#if defined(__PTRAUTH__)
// INTRIN: has_ptrauth_intrinsics
void has_ptrauth_intrinsics() {}
#else
diff --git a/clang/test/Sema/ptrauth-qualifier.c b/clang/test/Sema/ptrauth-qualifier.c
index 5d932b724f07a..3e568ce9f37e3 100644
--- a/clang/test/Sema/ptrauth-qualifier.c
+++ b/clang/test/Sema/ptrauth-qualifier.c
@@ -1,13 +1,25 @@
-// RUN: %clang_cc1 -triple arm64-apple-ios -std=c23 -fsyntax-only -verify -fptrauth-intrinsics %s
+// RUN: %clang_cc1 -triple arm64-apple-ios -DIS_DARWIN -std=c23 -fsyntax-only -verify -fptrauth-intrinsics %s
// RUN: %clang_cc1 -triple aarch64-linux-gnu -std=c23 -fsyntax-only -verify -fptrauth-intrinsics %s
-#if !__has_extension(ptrauth_qualifier)
+#if defined(IS_DARWIN) && !__has_extension(ptrauth_qualifier)
// This error means that the __ptrauth qualifier availability test says that it
// is not available. This error is not expected in the output, if it is seen
// there is a feature detection regression.
#error __ptrauth qualifier not enabled
#endif
+#if defined(IS_DARWIN) && !__has_feature(ptrauth_qualifier)
+// This error means that the __has_feature test for ptrauth_qualifier has
+// failed, despite it being expected on darwin.
+#error __ptrauth qualifier not enabled
+#elif !defined(IS_DARWIN) && (__has_feature(ptrauth_qualifier) || __has_extension(ptrauth_qualifier))
+#error ptrauth_qualifier labeled a feature on a non-darwin platform
+#endif
+
+#if !defined (__PTRAUTH__)
+#error __PTRAUTH__ test macro not defined when ptrauth is enabled
+#endif
+
#if __aarch64__
#define VALID_CODE_KEY 0
#define VALID_DATA_KEY 2
diff --git a/clang/test/SemaObjC/ptrauth-qualifier.m b/clang/test/SemaObjC/ptrauth-qualifier.m
index 74bbe6f09899b..67a73bbe45777 100644
--- a/clang/test/SemaObjC/ptrauth-qualifier.m
+++ b/clang/test/SemaObjC/ptrauth-qualifier.m
@@ -1,13 +1,25 @@
-// RUN: %clang_cc1 -triple arm64-apple-ios -fsyntax-only -verify -fptrauth-intrinsics %s
+// RUN: %clang_cc1 -triple arm64-apple-ios -DIS_DARWIN -fsyntax-only -verify -fptrauth-intrinsics %s
// RUN: %clang_cc1 -triple aarch64-linux-gnu -fsyntax-only -verify -fptrauth-intrinsics %s
-#if !__has_extension(ptrauth_qualifier)
+#if defined(IS_DARWIN) && !__has_extension(ptrauth_qualifier)
// This error means that the __ptrauth qualifier availability test says that it
// is not available. This error is not expected in the output, if it is seen
// there is a feature detection regression.
#error __ptrauth qualifier not enabled
#endif
+#if defined(IS_DARWIN) && !__has_feature(ptrauth_qualifier)
+// This error means that the __has_feature test for ptrauth_qualifier has
+// failed, despite it being expected on darwin.
+#error __ptrauth qualifier not enabled
+#elif !defined(IS_DARWIN) && (__has_feature(ptrauth_qualifier) || __has_extension(ptrauth_qualifier))
+#error ptrauth_qualifier labeled a feature on a non-darwin platform
+#endif
+
+#if !defined (__PTRAUTH__)
+#error __PTRAUTH__ test macro not defined when ptrauth is enabled
+#endif
+
@interface Foo
// expected-warning@-1 {{class 'Foo' defined without specifying a base class}}
// expected-note@-2 {{add a super class to fix this problem}}
|
cor3ntin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a blurb in the release note saying _has_feature(ptrauth_qualifier) is deprecated and PTRAUTH should be used instead?
clang/lib/Lex/PPMacroExpansion.cpp
Outdated
| /// specified by the identifier as a standard language feature. | ||
| static bool HasFeature(const Preprocessor &PP, StringRef Feature) { | ||
| const LangOptions &LangOpts = PP.getLangOpts(); | ||
| const TargetInfo &Target = PP.getTargetInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in the FEATURE() macro below for the darwin restriction on the feature checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment at the top of Features.def to explain this variable is also available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that this increases the number of decls required for FEATURE, and I'm just pulling the target off PP anyway, so I'll just inline that in the expression.
| if (LangOpts.PointerAuthIntrinsics) | ||
| Builder.defineMacro("__PTRAUTH__"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we synchronize with GCC here (or does gcc not support this feature at all yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe gcc has any support as yet? I recall talking to one gcc person in Sofia but it didn't sound like it's high priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so there's an existing __ARM_FEATURE_PAUTH but that does not cover the language features.
Naming-wise do we want something along the lines of __ARM_FEATURE_PAUTH_LANG_SUPPORT__ ?
__ARM_FEATURE_PAUTH only indicates the target supports the features -- gcc doesn't provide anything more than that afaict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we have __ARM_FEATURE_PAUTH already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also __ARM_FEATURE_PAC_DEFAULT, apparently. But this seems fairly target-related.
I'm fine with __PTRAUTH__ (or __PTRAUTH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @AaronBallman had an opinion on the __ suffix - I am absolutely indifferent here and as demonstrated am terrible at names :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with __PTRAUTH__; do we want it to expand to a particular value so we can change the value to have the macro mean a different feature set is enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the complexity of the ptrauth abi surface area means that I don't think any single value can really represent it - there's significant variation for us in userspace vs kernel vs other constrained environments vs what the linux folk are doing.
I think any dev who does need to work at this level will need to be directly detecting each single mode that is active
| FEATURE(scudo, LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo)) | ||
| FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics) | ||
| EXTENSION(ptrauth_qualifier, LangOpts.PointerAuthIntrinsics) | ||
| FEATURE(ptrauth_intrinsics, LangOpts.PointerAuthIntrinsics && Target.getTriple().isOSDarwin()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both of these are clearly identical so I figured making them both darwin only given PTRAUTH serves the purpose for everything
|
// ptrauth.h
#if !defined(__PTRAUTH__) && whatever_backwards_compat_check_darwin_needs
#define __PTRAUTH__
#endif
#ifdef __PTRAUTH__
#ifdef __PTRAUTH_ABI_PTRAUTH_CALLS__
#define __ptrauth_internal_has_abi_define_ptrauth_calls__
#endif
#ifdef __PTRAUTH_ABI_PTRAUTH_RETURNS__
#define __ptrauth_internal_has_abi_define_ptrauth_returns__
#endif
#define __has_ptrauth_feature(feature_name) \
__has_feature(feature_name) || defined(_____ptrauth_internal_has_abi_define_##feature_name##__)
#else
#define __has_ptrauth_feature(feature_name) 0
#endif
#if defined(__PTRAUTH__)
... all the usual defines ...
#else
.. the fallback no op defines ...
#endif
|
|
I realized that this can't be made to work nicely and automatically no matter how much I would like it to as of course the macro based version of |
| if (LangOpts.PointerAuthIntrinsics) | ||
| Builder.defineMacro("__PTRAUTH__"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with __PTRAUTH__; do we want it to expand to a particular value so we can change the value to have the macro mean a different feature set is enabled?
clang/lib/Lex/PPMacroExpansion.cpp
Outdated
| /// specified by the identifier as a standard language feature. | ||
| static bool HasFeature(const Preprocessor &PP, StringRef Feature) { | ||
| const LangOptions &LangOpts = PP.getLangOpts(); | ||
| const TargetInfo &Target = PP.getTargetInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment at the top of Features.def to explain this variable is also available.
Co-authored-by: Aaron Ballman <[email protected]>
clang/docs/ReleaseNotes.rst
Outdated
| Deprecated Compiler Flags | ||
| ------------------------- | ||
|
|
||
| - Use of ``__has_feature`` to detect the ``ptrauth_qualifier`` and ``ptrauth_intrinsics`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really a deprecated compiler flag. Perhaps we should move this to the non-comprehensive changes section? Or make up a new section for deprecated functionality?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo release note nit.
…lifier-and-intrinsics
…lifier-and-intrinsics
…vailable on Darwin (llvm#153912) For backwards compatibility reasons the `ptrauth_qualifier` and `ptrauth_intrinsic` features need to be testable with `__has_feature()` on Apple platforms, but for other platforms this backwards compatibility issue does not exist. This PR resolves these issues by making the `ptrauth_qualifier` and `ptrauth_intrinsic` tests conditional upon a darwin target. This also allows us to revert the ptrauth_qualifier check from an extension to a feature test again, as is required on these platforms. At the same time we introduce a new predefined macro `__PTRAUTH__` that answers the same question as `__has_feature(ptrauth_qualifier)` and `__has_feature(ptrauth_intrinsic)` as those tests are synonymous and only exist separately for compatibility reasons. The requirement to test for the `__PTRAUTH__` macro also resolves the hazard presented by mixing the `ptrauth_qualifier` flag (that impacts ABI and security policies) with `-pedantics-errors`, which makes `__has_extension` return false for all extensions. --------- Co-authored-by: Aaron Ballman <[email protected]> (cherry picked from commit 624b724)
In llvm/llvm-project#153912, usage of `__has_feature(ptrauth_intrinsics)` was restricted to Darwin-only, while `__PTRAUTH__` macro was introduced instead. This patch makes use of a preprocessor check against `__PTRAUTH__` macro in addition to previously used `__has_feature(ptrauth_intrinsics)`. The latter check is kept for backward compatibility.
For backwards compatibility reasons the
ptrauth_qualifierandptrauth_intrinsicfeatures need to be testable with__has_feature()on Apple platforms, but for other platforms this backwards compatibility issue does not exist.This PR resolves these issues by making the
ptrauth_qualifierandptrauth_intrinsictests conditional upon a darwin target. This also allows us to revert the ptrauth_qualifier check from an extension to a feature test again, as is required on these platforms.At the same time we introduce a new predefined macro
__PTRAUTH__that answers the same question as__has_feature(ptrauth_qualifier)and__has_feature(ptrauth_intrinsic)as those tests are synonymous and only exist separately for compatibility reasons.The requirement to test for the
__PTRAUTH__macro also resolves the hazard presented by mixing theptrauth_qualifierflag (that impacts ABI and security policies) with-pedantics-errors, which makes__has_extensionreturn false for all extensions.